Enable Labels in Container Config #750
Conversation
- only exposes in core the opportunity to add labels - this PR does NOT - pass through labels from a base image (should be in followup) - enable frontend labels access (should be in followup)
loosebazooka
requested review from
briandealwis,
TadCordle,
coollog and
chanseokoh
| @@ -98,6 +99,23 @@ public Builder setExposedPorts(@Nullable List<Port> exposedPorts) { | |||
| return this; | |||
| } | |||
|
|
|||
| /** | |||
| * Sets the container's exposed ports. | |||
There was a problem hiding this comment.
Leftover copy-pasted comment?
There was a problem hiding this comment.
copy-pasta strikes again, I'll fix these.
| } | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the items in the "ExposedPorts" field in the container configuration. |
| Map<String, String> badLabels = new HashMap<>(); | ||
| badLabels.put("label-key", null); | ||
| BuildConfiguration.builder(Mockito.mock(BuildLogger.class)) | ||
| .setContainerConfiguration(ContainerConfiguration.builder().setLabels(badLabels).build()); |
There was a problem hiding this comment.
The try block should only be focused on the method being tested. I actually think BuildConfiguration.builder() doesn't play a role. Maybe
Map<String, String> badLabels = ...
badLabels.put(...)
Builder builder = ContainerConfiguration.builder();
try {
builder.setLabels(badLabels);
Assert.fail(...);
} catch (...) {There was a problem hiding this comment.
hrmm... looks like we might need to cleanup this whole test class :\
| @@ -110,15 +111,26 @@ | |||
| * @param exposedPorts the list of exposed ports to add | |||
| * @return this | |||
| */ | |||
| public Builder<T> setExposedPorts(@Nullable ImmutableList<Port> exposedPorts) { | |||
| public Builder<T> setExposedPorts(@Nullable List<Port> exposedPorts) { | |||
| if (exposedPorts == null) { | |||
There was a problem hiding this comment.
I guess we could do the same thing here as with labels and use a ternary operator.
There was a problem hiding this comment.
Yeah, I was going to, but figured it should be in a followup. This ImmutableList -> List change probably shouldn't be part of this PR either. I might go through and do some cleanup after this.
part of #751